Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor nf-core modules commands #1124

Merged
merged 12 commits into from
Jun 28, 2021

Conversation

ErikDanielsson
Copy link
Contributor

@ErikDanielsson ErikDanielsson commented Jun 23, 2021

Refactored pipeline_modules.py into one file file per nf-core modules command.

  • Removed pipeline_modules.py
  • Created base class ModuleCommand for modules commands to inherit from.
  • Created classes for list, install and remove commands.

The modules commands are all now found in <command>.py.

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

@github-actions

This comment has been minimized.

@ErikDanielsson ErikDanielsson changed the base branch from master to dev June 23, 2021 09:22
@codecov
Copy link

codecov bot commented Jun 23, 2021

Codecov Report

Merging #1124 (c1dce02) into dev (c196325) will increase coverage by 0.16%.
The diff coverage is 63.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1124      +/-   ##
==========================================
+ Coverage   70.41%   70.58%   +0.16%     
==========================================
  Files          45       48       +3     
  Lines        4797     4827      +30     
==========================================
+ Hits         3378     3407      +29     
- Misses       1419     1420       +1     
Impacted Files Coverage Δ
nf_core/__main__.py 62.11% <0.00%> (+1.13%) ⬆️
nf_core/modules/test_yml_builder.py 45.94% <50.00%> (ø)
nf_core/modules/install.py 53.38% <53.38%> (ø)
nf_core/modules/module_utils.py 76.08% <60.00%> (+12.98%) ⬆️
nf_core/modules/list.py 61.11% <61.11%> (ø)
nf_core/modules/modules_command.py 78.20% <78.20%> (ø)
nf_core/modules/remove.py 81.25% <81.25%> (ø)
nf_core/modules/__init__.py 100.00% <100.00%> (ø)
nf_core/modules/lint/__init__.py 75.96% <100.00%> (+0.84%) ⬆️
nf_core/modules/lint/module_changes.py 78.26% <0.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c196325...c1dce02. Read the comment docs.

@KevinMenden
Copy link
Contributor

Hi @ErikDanielsson , overall I think this is a nice idea!

One thing that is a bit tricky with the modules commands though is that some of them (mostly lint at the moment) can be run on pipelines and on the nf-core/modules repository. So I would suggest to add a repo_type field to the base class and a method to identify the repo class (we have this in the lint code, just need to copy+paste and adapt a little). And then other methods such has has_valid_pipeline will need to be adapted accordingly.

But other than that I think this is nice! But would be nice to hear what @ewels thinks about this :)

@ErikDanielsson
Copy link
Contributor Author

Thanks, I'll take a look at the lint code.

Copy link
Contributor

@KevinMenden KevinMenden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my phone, didn't get very far with the review sorry. But concept sounds great! I'm happy to merge if it LGTM you guys. Especially as high risk for merge conflicts.

def install(self, module):
if self.repo_type == "modules":
log.error("You cannot install a module in a clone of nf-core/modules")
sys.exit(1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we raise an exception instead and catch it in the cli code? I prefer not to have exits anywhere but there if possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other checks just return false. So that would also be fine.

@ErikDanielsson ErikDanielsson merged commit 03f1127 into nf-core:dev Jun 28, 2021
@ErikDanielsson ErikDanielsson deleted the refactor-module-commands branch July 25, 2022 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants